-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat!: change the default of --forbid-only to check for process.env.CI #5496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thanks @RobinVdBroeck for this one :) unfortunately from what I'm seeing, our Curious if @mochajs/maintenance-crew has any insights here, otherwise this may sit for a bit while I work on other bugfixes and think about this one |
|
Yeah, I think it's fine to omit testing for this one. The behavioral change is one line. Tests would be ideal but 🤷 it's old code locked into a design pattern we don't control. |
2dc2aa0 to
829f504
Compare
829f504 to
79e2adf
Compare
|
I've pushed 2 new commits:
|
|
Hi @RobinVdBroeck would you mind resolving the new conflicts? They should just be format fixes, The new message looks good to me, thank you |
PR Checklist
status: accepting prsOverview
I've changed the default value of
--forbid-onlyto be the truthy value of process.env.CI.Since this is a major change, I've added documentation for the behaviour before v12 (the next major release) and after. Let me know if this is not how docs are written for mocha, and how I should change it to be inline with mocha's standards.
I could use some pointers on where to add tests. I've only found integration level tests for testing CLI flags, but that seems kinda overkill for adding a default? Should I write an unit test somewhere, and if so, where?